Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[MM-53457] Standalone client #115

Merged
merged 30 commits into from
Nov 21, 2023
Merged

[MM-53457] Standalone client #115

merged 30 commits into from
Nov 21, 2023

Conversation

streamer45
Copy link
Contributor

Summary

PR implements a standalone Golang client for Calls. This is mostly based of the official one included in the plugin. The implementation abstracts away all the logic necessary to successfully join a call given a SiteURL, AuthToken and ChannelID.

This includes:

  • WebSocket client implementation.
  • WebRTC client implementation.
  • Any Calls plugin specific API logic (e.g. join message, reconnect support, etc).

There are however some key differences:

  • The WebRTC API is the one provided by pion which comes with its own set of limitations (e.g. perfect negotiation is not yet supported).
  • The client is currently receive-only, purely useful for receiving tracks since that's the main use case. I don't exclude extending this to be a full functional client but not a priority. If we ever do that it may be interesting to replace the one used for load-testing with this.

This also comes with some additional limitations and potential TODOs:

  • We are not reading the plugin's config which means we are not setting any ICE server configs. As the client is mostly meant to connect internally they shouldn't be necessary but something that could be added to make it a little closer to a real implementation.
  • We are not implementing the MM reliable websocket functionality. This is partly because it doesn't really work in HA clusters so not sure if it's worth adding for this use case.

To note, this PR also includes a very basic e2e pipeline as tests are connecting against a running version of the plugin compiled with the latest rtcd changes. This could also serve as a good base to implement more tests that check general functionality.

Ticket Link

https://mattermost.atlassian.net/browse/MM-53457

@streamer45 streamer45 added 2: Dev Review Requires review by a core committer Do Not Merge Should not be merged until this label is removed labels Jul 12, 2023
@streamer45 streamer45 requested a review from cpoile July 12, 2023 23:24
@streamer45 streamer45 self-assigned this Jul 12, 2023
cpoile
cpoile previously approved these changes Jul 21, 2023
Copy link
Member

@cpoile cpoile left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks very clean, nicely done. Nothing jumps out as a need to fix, and I need to get more familiar with pion to know if you're doing anything out of the ordinary there. But I'll assume you're not. 👍

@streamer45 streamer45 added 3: Reviews Complete All reviewers have approved the pull request and removed 2: Dev Review Requires review by a core committer labels Jul 24, 2023
@streamer45
Copy link
Contributor Author

Added a couple of extra commits to facilitate the transcription use case, mainly being able to react to user disconnecting and call ending.

@streamer45 streamer45 requested a review from cpoile August 17, 2023 16:48
cpoile
cpoile previously approved these changes Aug 18, 2023
Copy link
Member

@cpoile cpoile left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, non-blocking comments

cpoile
cpoile previously approved these changes Aug 28, 2023
Copy link
Member

@cpoile cpoile left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@streamer45 streamer45 added this to the v0.12.0 milestone Aug 31, 2023
@streamer45 streamer45 removed the Do Not Merge Should not be merged until this label is removed label Aug 31, 2023
@streamer45 streamer45 modified the milestones: v0.12.0, v0.13.0 Nov 9, 2023
jupenur
jupenur previously approved these changes Nov 15, 2023
* Fix race condition on stop

* Wait for signaling goroutines to exit before returning on close
@streamer45 streamer45 merged commit 6a56862 into master Nov 21, 2023
@streamer45 streamer45 deleted the MM-53457 branch November 21, 2023 17:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3: Reviews Complete All reviewers have approved the pull request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants